[Refactor] New rule parser to support AST#104
Conversation
colinthebomb1
force-pushed
the
colin/rule-parser-parse-v2
branch
from
bfc2e15 to
0fd8c98
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces RuleParserV2, a new rule parsing implementation that generates AST node structures instead of JSON, along with a comprehensive refactoring of variable node classes.
Changes:
- New RuleParserV2 module that parses rules into AST structures with ElementVariableNode and SetVariableNode for rule variables
- Renamed VarNode → ElementVariableNode and VarSetNode → SetVariableNode throughout the codebase for semantic clarity
- Updated internal token naming from V/VL to EV/SV (ElementVariable/SetVariable) to align with new v2 semantics
- Comprehensive test coverage (745 lines) for RuleParserV2 including parametrized tests over entire rule catalog
- Code cleanup in test_rule_parser.py (import consolidation and formatting)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/rule_parser_v2.py | New RuleParserV2 implementation with 6-step pipeline for rule parsing and AST generation |
| tests/test_rule_parser_v2.py | Comprehensive test suite covering extendToFullSQL, replaceVars, parsing, variable substitution, and rule catalog validation |
| core/ast/node.py | Renamed VarNode/VarSetNode to ElementVariableNode/SetVariableNode with updated docstrings |
| core/ast/init.py | Updated exports to use new class names |
| tests/test_rule_parser.py | Import consolidation and code cleanup (formatting and indentation fixes) |
| tests/test_ast.py | Updated imports to use new ElementVariableNode/SetVariableNode names |
| tests/ast_util.py | Updated imports and comments to reference new variable node class names |
| core/query_parser.py | Updated imports and TODO comment to reference new class names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HazelYuAhiru
left a comment
HazelYuAhiru
left a comment
There was a problem hiding this comment.
Overall LGTM! Just with a small comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
baiqiushi
left a comment
baiqiushi
left a comment
There was a problem hiding this comment.
Review Summary
Overall the PR is well-structured — clean renames, solid test coverage (774 lines), and all prior review threads resolved. One correctness issue found:
🐛 Variable namespace collision in replaceVars (core/rule_parser_v2.py:194-197)
replaceVars processes set variables (<<x>>) before element variables (<x>), but both passes share the same mapping dict. When a rule uses the same variable name for both types (e.g., <<x>> and <x>), the element variable pass is silently skipped because if var not in mapping (line 181) sees the name already registered from the set variable pass.
Example:
Input: "SELECT <<x>>, <x> FROM t"
Expected: "SELECT SV001, EV001 FROM t"
Actual: "SELECT SV001, <x> FROM t" ← <x> left as literal text
The leftover <x> is invalid SQL and will cause the downstream mo_sql_parsing step to fail or produce a wrong AST.
Suggested fix: Key the mapping by (var_name, var_type) tuples instead of bare name, or use separate mapping dicts per variable type and merge them afterward. The downstream _substitute_placeholders (which consumes the reverse mapping) would also need updating accordingly.
baiqiushi
left a comment
baiqiushi
left a comment
There was a problem hiding this comment.
The AI reviews can be resolved by assuming the variable name cannot be overlapped even between different variable types.
Summary
This PR introduces and stabilizes RuleParser v2 along with dedicated v2 test coverage.
Highlights
<x>)<<y>>)test_rule_parser_v2.py.Testing
tests/test_rule_parser_v2.py